-
Notifications
You must be signed in to change notification settings - Fork 195
Ohadn/blake2s last block opcode vm #1932
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Benchmark Results for unmodified programs 🚀
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1932 +/- ##
=======================================
Coverage 96.39% 96.39%
=======================================
Files 102 102
Lines 41483 41566 +83
=======================================
+ Hits 39987 40068 +81
- Misses 1496 1498 +2 ☔ View full report in Codecov by Sentry. |
bcf7dc5
to
4919048
Compare
2ce852d
to
d25cef1
Compare
4919048
to
0260570
Compare
1c031e4
to
de91ec1
Compare
b26563b
to
246904f
Compare
0260570
to
7b7d956
Compare
246904f
to
4490aab
Compare
4a7be69
to
62c28ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 9 files at r1, 4 of 4 files at r6, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @Oppen, @pefontana, and @YairVaknin-starkware)
cairo_programs/stwo_exclusive_programs/blake2s_opcode_test.cairo
line 138 at r6 (raw file):
let offset1 = (2**15)-4; let offset2 = (2**15)-3; static_assert dst == [fp -5];
space after the -
cairo_programs/stwo_exclusive_programs/blake2s_opcode_test.cairo
line 161 at r6 (raw file):
let flag_num = flag_dst_base_fp+flag_op0_base_fp*(2**1)+flag_op1_imm*(2**2)+flag_op1_base_fp*(2**3); let blake2s_opcode_extension_num = 1; let blake2s_last_block_opcode_extension_num =2;
missing space
cairo_programs/stwo_exclusive_programs/blake2s_opcode_test.cairo
line 167 at r6 (raw file):
static_assert blake2s_last_block_instruction_num==18449981025204076539; // Write the instruction to [pc] and point [ap+1] to the designated output.
how did the +1
pop up? if the reason is hidden in the if
statement I would replace it with the plane jmp
syntax
62c28ae
to
4f2ec51
Compare
0e3fb2c
to
330f44d
Compare
4f2ec51
to
261b2f7
Compare
d707312
to
2190f0d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 11 files reviewed, 4 unresolved discussions (waiting on @DavidLevitGurevich, @fmoletta, @gabrielbosio, @igaray, @juanbono, @Oppen, @pefontana, and @YairVaknin-starkware)
cairo_programs/stwo_exclusive_programs/blake2s_opcode_test.cairo
line 138 at r6 (raw file):
Previously, DavidLevitGurevich wrote…
space after the
-
Done.
cairo_programs/stwo_exclusive_programs/blake2s_opcode_test.cairo
line 161 at r6 (raw file):
Previously, DavidLevitGurevich wrote…
missing space
Done.
cairo_programs/stwo_exclusive_programs/blake2s_opcode_test.cairo
line 167 at r6 (raw file):
Previously, DavidLevitGurevich wrote…
how did the
+1
pop up? if the reason is hidden in theif
statement I would replace it with the planejmp
syntax
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 9 files at r7.
Reviewable status: 5 of 11 files reviewed, 1 unresolved discussion (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @Oppen, @pefontana, and @YairVaknin-starkware)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 5 of 11 files reviewed, 1 unresolved discussion (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @Oppen, @pefontana, and @YairVaknin-starkware)
2190f0d
to
2a632d3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r8, all commit messages.
Reviewable status: 7 of 11 files reviewed, 1 unresolved discussion (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @Oppen, @pefontana, and @YairVaknin-starkware)
d0711e3
to
c52927b
Compare
63e63fa
to
b330037
Compare
vm/src/vm/vm_core.rs
Outdated
&message, | ||
counter, | ||
0, | ||
if !is_last_block { 0 } else { 0xffffffff }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please extract this logic into a variable next to the definitions of state
and message
(lines 481-497)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
if (opcode_extension == OpcodeExtension::Blake | ||
|| opcode_extension == OpcodeExtension::BlakeFinalize) | ||
&& (opcode != Opcode::NOp | ||
|| (op1_addr != Op1Addr::FP && op1_addr != Op1Addr::AP) | ||
|| res != Res::Op1 | ||
|| pc_update != PcUpdate::Regular | ||
|| (ap_update_num != 0 && ap_update_num != 2)) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would extract these conditionals into a variable to separate the opcode_extension
checks of the flags check. Something like:
if (opcode_extension == OpcodeExtension::Blake | |
|| opcode_extension == OpcodeExtension::BlakeFinalize) | |
&& (opcode != Opcode::NOp | |
|| (op1_addr != Op1Addr::FP && op1_addr != Op1Addr::AP) | |
|| res != Res::Op1 | |
|| pc_update != PcUpdate::Regular | |
|| (ap_update_num != 0 && ap_update_num != 2)) | |
{ | |
let are_blake_flags_invalid = opcode != Opcode::NOp | |
|| (op1_addr != Op1Addr::FP && op1_addr != Op1Addr::AP) | |
|| res != Res::Op1 | |
|| pc_update != PcUpdate::Regular | |
|| (ap_update_num != 0 && ap_update_num != 2; | |
if (opcode_extension == OpcodeExtension::Blake | |
|| opcode_extension == OpcodeExtension::BlakeFinalize) | |
&& are_opcodes_flags_invalid) | |
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
b330037
to
cee3133
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 5 of 11 files reviewed, 3 unresolved discussions (waiting on @DavidLevitGurevich, @fmoletta, @gabrielbosio, @igaray, @juanbono, @Oppen, @pefontana, @YairVaknin-starkware, and @yuvalsw)
vm/src/vm/vm_core.rs
Outdated
&message, | ||
counter, | ||
0, | ||
if !is_last_block { 0 } else { 0xffffffff }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
if (opcode_extension == OpcodeExtension::Blake | ||
|| opcode_extension == OpcodeExtension::BlakeFinalize) | ||
&& (opcode != Opcode::NOp | ||
|| (op1_addr != Op1Addr::FP && op1_addr != Op1Addr::AP) | ||
|| res != Res::Op1 | ||
|| pc_update != PcUpdate::Regular | ||
|| (ap_update_num != 0 && ap_update_num != 2)) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 9 files at r7, 2 of 3 files at r9, 2 of 2 files at r10, all commit messages.
Reviewable status: 10 of 11 files reviewed, 3 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @Oppen, @pefontana, @YairVaknin-starkware, and @yuvalsw)
Blake2sLastBlock opcode runner
Description
Adding the opcode Blake2sLastBlock to the VM.
Expects op0 to be a pointer to a sequence of 9 felts and and op1 to be a pointer to a sequence of 16 felts.
Said felts should represent u32 integers, i.e. have value of at most 2**32-1.
The first 8 felts of op0 represent a state, the 9th represents the counter and the 10th represents counter+n_bytes i.e. the total length of the entire message.
The 16 felts of op1 represent a message.
The "output" consists of 8 felts representing u32 numbers of the Blake2s compression of the last block.
dst should be a pointer, it points to a sequence of 8 cells which each should either be uninitialised or already contain a value matching that of the output at the same index.
The opcode inserts the aforementioned output into the 8 cells [dst], [dst+1], ... [dst+7] (and yields an error if one of said cells already contains a value differing from the output).
Currently Blake2sLastBlock has opcode_num 16, meaning encoded_instr is expanded to 128 bits with the 63 most significant bits expected to be 0 (otherwise yield an error).
The motivation is that it has been decided at Starkware that Blake2sLastBlock is to be implemented at an opcode, thus it needs to be supported by the runner.
Checklist
This change is